-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Proposal: Allow overwriting static utilities that have a namespace #18056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Going to retire this for now |
Actually still like this better than the alternatives. Let me see if an LLM can figure out all utilities without too much hassle. |
b3e8e4d
to
531201b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Got a few questions and suggestions related to.. suggestions to simplify this a bit more.
} | ||
|
||
.col-start-auto { | ||
grid-column: var(--grid-column-start-auto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this should be solved by #18907
border-top-left-radius: 3.40282e38px; | ||
border-top-right-radius: 3.40282e38px; | ||
border-top-left-radius: var(--radius-full); | ||
border-top-right-radius: var(--radius-full); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
.backdrop-blur-none { | ||
--tw-backdrop-blur: blur(var(--backdrop-blur-none)); | ||
-webkit-backdrop-filter: var(--tw-backdrop-blur, ) var(--tw-backdrop-brightness, ) var(--tw-backdrop-contrast, ) var(--tw-backdrop-grayscale, ) var(--tw-backdrop-hue-rotate, ) var(--tw-backdrop-invert, ) var(--tw-backdrop-opacity, ) var(--tw-backdrop-saturate, ) var(--tw-backdrop-sepia, ); | ||
backdrop-filter: var(--tw-backdrop-blur, ) var(--tw-backdrop-brightness, ) var(--tw-backdrop-contrast, ) var(--tw-backdrop-grayscale, ) var(--tw-backdrop-hue-rotate, ) var(--tw-backdrop-invert, ) var(--tw-backdrop-opacity, ) var(--tw-backdrop-saturate, ) var(--tw-backdrop-sepia, ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but this output is crazy and made me think about something... I wonder if we can be a bit smarter here and optimize the output based on what you used.
So if you only use blur
that we get rid of all the other variables used here.
If you have a utility that sets some backdrop opacity, or if we detect that --tw-backdrop-saturate
is used, that we start including it in the output.
Food for thought 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting, but unfortunately would be a breaking change if people used runtime to define these variables right? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, but we also consider those --tw-*
variables as private-ish anyway 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah fair I guess 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be the first case of the presence of other utilities affecting the generated CSS for different utilities. Would also need to invalidate the internal AST caches for those utilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it does feel like a bit of a stretch, especially in regard to the changes in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thecrypticace Yep but I like to think about it in a different way. Similar to how we define CSS variables and remove them when they are not used, or similar to how we only emit CSS for used utilities today.
The CSS variables are also only emitted based on the presence of other utilities (except for @theme static
).
If you think about it like this, then it could be part of the optimization step where we remove certain @property
at-rules or used var(--tw-*)
in declarations. Might not be as easy to generalize this but it wouldn't touch the cached AST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it does feel like a bit of a stretch, especially in regard to the changes in this PR.
Hence the Not related to this PR
part, just noticed it while looking over the diff 😅
So definitely something that should not be part of this PR. Please ignore all aforementioned comments 🙈.
staticValues: { | ||
center: [decl('transform-origin', 'center')], | ||
top: [decl('transform-origin', 'top')], | ||
'top-right': [decl('transform-origin', '100% 0')], | ||
right: [decl('transform-origin', '100%')], | ||
'bottom-right': [decl('transform-origin', '100% 100%')], | ||
bottom: [decl('transform-origin', 'bottom')], | ||
'bottom-left': [decl('transform-origin', '0 100%')], | ||
left: [decl('transform-origin', '0')], | ||
'top-left': [decl('transform-origin', '0 0')], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost wish this was a Map so all the keys would have quotes around them 😂
// Ensure discoverability for `col-auto` after converting to fallback | ||
suggest('col', () => [{ values: ['auto'], valueThemeKeys: ['--grid-column'] }]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can make these suggestions more automatic as part of the functionalUtility
helper because we know the root, we know the values (keys of staticValues
) and we know the valueThemeKeys
.
Should simplify some of the additional code and ensure everything is in sync when we update some static values. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't provide a suggestion to code that didn't change (thanks GitHub!), but something like this should work I think:
if (desc.staticValues) {
let values = Object.keys(desc.staticValues)
suggest(classRoot, () => [{ values, valueThemeKeys: desc.themeKeys ?? [] }])
}
at the end of the functionalUtility
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm, right now though, the functionalUtility
does not provide any suggestions on its own and I think that's a good thing: If we ever decide to double-down on the browser build, we'd want a convenient way to compile the suggestions away and then this current setup will become useful. That said for the previous staticUtility()
helper, suggestions are provided by default so it wasn't very consistent to begin with so I'm fine with either approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now though, the
functionalUtility
does not provide any suggestions on its own
What do you mean by this exactly? We do have this already:
tailwindcss/packages/tailwindcss/src/utilities.ts
Lines 449 to 456 in 340b59d
suggest(classRoot, () => [ | |
{ | |
supportsNegative: desc.supportsNegative, | |
valueThemeKeys: desc.themeKeys ?? [], | |
hasDefaultValue: desc.defaultValue !== undefined && desc.defaultValue !== null, | |
supportsFractions: desc.supportsFractions, | |
}, | |
]) |
Which is part of the functionalUtility
function when being called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, interesting! Then I guess I am wrong and it should just work. no idea why namespaces keys aren't added automatically then too, though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RobinMalfait updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more things that can be deleted with that latest change.
A few things after this:
- Need to update tests because of the fix in #18907
- Needs a changelog entry
a846126
to
c6ad58f
Compare
…ppress generation when namespaced; keep IntelliSense unchanged; rename fallbacks->staticValues; add targeted override tests
Co-authored-by: Robin Malfait <[email protected]>
…stions non-negative and merge with existing groups
c6ad58f
to
88b4541
Compare
This PR attempts to move static utilities that are overwriteable by a theme value to be a fallback rather than a conflicting implementation. The idea is to allow a theme value to take presedence over that static utility and cause it not to generate.
For example, when overwriting the
--radius-full
variant, it should ensure that the defaultrounded-full
no longer emits thecalc(infinity * 1px)
declaration:This allows anyone who wants
--radius-full
to be a CSS variable to simply define it in their theme:The idea is to extend this pattern across all functional utilities that also have static utilities that can collide with the namespace. This gives users more control over what they want as CSS variables when the defaults don't work for them, allowing them to resolve #16639 and #15115 in user space.
You may now find yourself thinking "but Philipp, why would someone want to be able to overwrite
--animate-none
.none
surely always will mean no animation" and I would agree but it's already possible right now anyways so this is not a new behavior! This PR just cleans up the generated output.